[SD-22]Updated hero image theme according header style #516

yeniatencio · 2024-10-01T08:30:10Z

Jira

https://digital-vic.atlassian.net/browse/SD-22
BE testing link: https://nginx-php.pr-1711.content-vic.sdp4.sdp.vic.gov.au/

Problem/Motivation

We want to have these scenarios for customised header:

Corner graphics - Light + Dark (Both options available and by default Light should be selected),
Full background image - Dark only (Only one option should be set)
CTA - Light only (Only one option should be set).
Default appearance - Light only (Only one option should be set)

Fix

Set field field_landing_page_hero_theme to mandatory.
Remove a patch for drupal/jsonapi_extras as new released was done and patch is not needed anymore.
Fix typo in label : Here image theme.
Updated behat test.
Updated tide publication module to make it work in publication and publication page content type.

Related PRs

Screenshots

TODO

MdNadimHossain

@yeniatencio use states instead of custom JS. See this example - https://github.com/dpc-sdp/tide_core/blob/develop/modules/tide_landing_page/tide_landing_page.module#L218

MdNadimHossain · 2024-10-04T06:18:24Z

modules/tide_landing_page/js/hero_image_theme.js

+      }
+    },
+  };
+})(Drupal);


We do not want use js if it is can be done without it. Read the SD, I have mentioned about conditional logic. You will find other examples in landing page where we have used conditional logic states. Take example from there and try to work it out using states instead of adding this JS please.

@MdNadimHossain agreed. I've already using conditional logic for enable/disable the field_landing_page_hero_theme as per the requirements. However, one of the requirements is to set the default value of one field based on the value of another field and the only Drupal States that can be applied to a form field element are:

enabled

disabled

required

optional

visible

invisible

checked

unchecked

expanded

collapsed

As you can notice the default value can't be changed using states. Also, all the examples in landing page module is for changing one of the states in the list above but a default value.
Therefore, that's why I ended up writing JS code for it. Hope it makes sense now.

MdNadimHossain

@yeniatencio I have suggested some suggestions. Please have a look into the revised version and it should work in the same way -

(function (Drupal) {
  "use strict";

  Drupal.behaviors.heroImageTheme = {
    attach: function (context, settings) {
      const imageTheme = context.querySelector(
        'select[data-drupal-selector^="edit-field-landing-page-hero-theme"]'
      );

      if (imageTheme !== null) {
        setImageTheme();

        const headerStyles = context.querySelectorAll(
          'input[data-drupal-selector^="edit-header-style-options"]'
        );

        if (headerStyles.length > 0) {
          headerStyles.forEach((style) => {
            style.addEventListener("change", setImageTheme);
          });
        }
      }

      function setImageTheme() {
        const selectedHeaderStyle = document.querySelector(
          'input[data-drupal-selector^="edit-header-style-options"]:checked'
        );

        if (!selectedHeaderStyle) {
          return; // Exit if no style option is selected.
        }

        let defaultHeaderStyle = selectedHeaderStyle.value;
        imageTheme.value = defaultHeaderStyle === "fullwidth" ? "dark" : "light";
      }
    },
  };
})(Drupal);

MdNadimHossain · 2024-10-08T06:02:38Z

modules/tide_landing_page/js/hero_image_theme.js

+
+  Drupal.behaviors.heroImageTheme = {
+    attach: function (context, settings) {
+      const imageTheme = document.querySelector(


Use context to ensure that the behavior only attaches to elements in the current page context.

MdNadimHossain · 2024-10-08T06:03:46Z

modules/tide_landing_page/js/hero_image_theme.js

+          'input[data-drupal-selector^="edit-header-style-options"]'
+        );
+
+        if (headerStyles !== null) {


The check if (headerStyles !== null) should instead check for headerStyles.length > 0. It’s better to check if any elements are found before adding listeners.

MdNadimHossain · 2024-10-08T06:07:25Z

modules/tide_landing_page/js/hero_image_theme.js

+      }
+
+      function setImageTheme() {
+        let defaultHeaderStyle = document.querySelector(


Add a guard clause to prevent this error in case no radio buttons are checked

MdNadimHossain · 2024-10-08T06:08:59Z

modules/tide_landing_page/js/hero_image_theme.js

+        if (headerStyles !== null) {
+          headerStyles.forEach((style) => {
+            style.addEventListener("change", () => {
+              setImageTheme();


Instead of repeatedly defining an anonymous function for style.addEventListener("change", ...), you can directly pass the reference to the setImageTheme function.

MdNadimHossain · 2024-10-08T06:13:08Z

modules/tide_landing_page/js/hero_image_theme.js

+
+        const headerStyles = document.querySelectorAll(
+          'input[data-drupal-selector^="edit-header-style-options"]'
+        );


The script accesses document multiple times. It’s a good practice to minimize global object access by caching elements that you reuse.

MdNadimHossain · 2024-10-08T06:13:43Z

modules/tide_landing_page/js/hero_image_theme.js

+        }
+      }
+
+      function setImageTheme() {


I would suggest moving the setImageTheme() function outside the main attach function for clarity and better separation of logic.

yeniatencio · 2024-10-08T21:16:16Z

@yeniatencio I have suggested some suggestions. Please have a look into the revised version and it should work in the same way -

(function (Drupal) {
  "use strict";

  Drupal.behaviors.heroImageTheme = {
    attach: function (context, settings) {
      const imageTheme = context.querySelector(
        'select[data-drupal-selector^="edit-field-landing-page-hero-theme"]'
      );

      if (imageTheme !== null) {
        setImageTheme();

        const headerStyles = context.querySelectorAll(
          'input[data-drupal-selector^="edit-header-style-options"]'
        );

        if (headerStyles.length > 0) {
          headerStyles.forEach((style) => {
            style.addEventListener("change", setImageTheme);
          });
        }
      }

      function setImageTheme() {
        const selectedHeaderStyle = document.querySelector(
          'input[data-drupal-selector^="edit-header-style-options"]:checked'
        );

        if (!selectedHeaderStyle) {
          return; // Exit if no style option is selected.
        }

        let defaultHeaderStyle = selectedHeaderStyle.value;
        imageTheme.value = defaultHeaderStyle === "fullwidth" ? "dark" : "light";
      }
    },
  };
})(Drupal);

Hi @MdNadimHossain , Thanks for the suggestions. I have updated my pr as you requested.

…ates when using ajax

yeniatencio · 2024-10-28T05:24:20Z

Hey @MdNadimHossain , I have updated my pr because there is a bug present when using states. In our case it happens when a hero image is added, then the hero image theme dropdown was being disabled.
For reference you can see here https://www.drupal.org/project/drupal/issues/1091852.
Now it works as requested:

vincent-gao · 2024-11-01T02:58:55Z

hey @yeniatencio
I believe this can be achieved using #ajax without adding custom JavaScript code. As @MdNadimHossain pointed out, we should avoid managing additional JS solely for visual enhancements. You can also implement a submit handler to validate the form if needed.

vincent-gao · 2024-11-01T03:00:31Z

for JS itself.

Limit the DOM operations within the context. https://github.com/dpc-sdp/tide_core/pull/516/files#diff-74b152131cfda4b1eb7d53850fae87f6de6abcbc57cd5143247a4b8480acc46aR25
Use the once method to avoid repeatedly binding event listeners when behaviors are executed multiple times

vincent-gao

hey @yeniatencio
Please review my comments and provide the test code for this new feature

vincent-gao · 2024-11-21T04:40:45Z

hey @yeniatencio , What I’ve changed is just a preliminary idea for your further work, and it’s not ready for a code review yet. I think there are still some places that need adjustments.

Remove the JS and its library.
Rename the function at https://github.com/dpc-sdp/tide_core/pull/516/files#diff-9e825e979aa05cc956b3d75ad6d8700172fa678536966ccb8d1c9ef1a96b1ecfR389, as the current name doesn't make sense.
With this approach, you may need to set a default value for the field, which will likely require an update hook.
Update the tests to cover the following scenarios:
1. Selecting a value from the field
2. Get excepted Hero image theme type
3. Save the node
4. Ensuring the Hero image theme displays the expected value

added logic for landing page

c7d45c8

yeniatencio self-assigned this Oct 1, 2024

yeniatencio added DO NOT MERGE DO NOT REVIEW labels Oct 1, 2024

yeniatencio added 3 commits October 1, 2024 21:02

Added hook update

30e903b

updated behat test

2c041b6

remove patch as new released has been launched

f3a0c31

yeniatencio added Needs review and removed DO NOT MERGE DO NOT REVIEW labels Oct 3, 2024

yeniatencio requested review from vincent-gao, anthony-malkoun and MdNadimHossain October 3, 2024 00:03

MdNadimHossain requested changes Oct 4, 2024

View reviewed changes

yeniatencio requested a review from MdNadimHossain October 7, 2024 08:30

yeniatencio changed the title ~~[SD-22]added logic for landing page~~ [SD-22]Updated hero image theme according header style Oct 8, 2024

MdNadimHossain requested changes Oct 8, 2024

View reviewed changes

Updated

9672fbc

yeniatencio requested a review from MdNadimHossain October 8, 2024 21:15

MdNadimHossain approved these changes Oct 8, 2024

View reviewed changes

yeniatencio added 2 commits October 23, 2024 19:17

resolving conflicts

af3eca9

Replaced drupal state condition with JS due to bug in Drupal core #st…

0e68315

…ates when using ajax

yeniatencio requested a review from MdNadimHossain October 28, 2024 05:24

Merge branch 'develop' into feature/SD-22-customised-options

79067b4

vincent-gao requested changes Nov 1, 2024

View reviewed changes

Merge branch 'develop' into feature/SD-22-customised-options

783369c

yeniatencio and others added 3 commits November 14, 2024 10:02

Merge branch 'develop' into feature/SD-22-customised-options

d6325e1

Merge branch 'develop' into feature/SD-22-customised-options

c25234d

Implement feedback

ab1735a

yeniatencio requested a review from vincent-gao November 17, 2024 22:18

vincent-gao and others added 4 commits November 18, 2024 18:45

test

4d1de87

fixed lint

294b0c6

added function description

eb3f802

added function description

c2996c2

yeniatencio and others added 10 commits November 25, 2024 20:02

removed js

6a00fe0

Merge branch 'develop' into feature/SD-22-customised-options

704ba8d

Update tide_landing_page.module

950b3f8

upgrade search api version

4524117

upgrade patch

ad83ea7

Merge branch 'develop' into feature/SD-22-customised-options

df394a3

added behat test for hero theme scenarios

9289343

fixed lint

66d8688

Merge branch 'develop' into feature/SD-22-customised-options

186915c

Merge branch 'develop' into feature/SD-22-customised-options

0852f96

yeniatencio mentioned this pull request Nov 26, 2024

[SD-22] Updated hero theme image #555

Open

Merge branch 'develop' into feature/SD-22-customised-options

a40650e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SD-22]Updated hero image theme according header style #516

[SD-22]Updated hero image theme according header style #516

yeniatencio commented Oct 1, 2024 •

edited

Loading

MdNadimHossain left a comment

MdNadimHossain Oct 4, 2024

yeniatencio Oct 7, 2024

MdNadimHossain left a comment

MdNadimHossain Oct 8, 2024

MdNadimHossain Oct 8, 2024

MdNadimHossain Oct 8, 2024

MdNadimHossain Oct 8, 2024

MdNadimHossain Oct 8, 2024

MdNadimHossain Oct 8, 2024

yeniatencio commented Oct 8, 2024

yeniatencio commented Oct 28, 2024

vincent-gao commented Nov 1, 2024

vincent-gao commented Nov 1, 2024

vincent-gao left a comment

vincent-gao commented Nov 21, 2024

[SD-22]Updated hero image theme according header style #516

Are you sure you want to change the base?

[SD-22]Updated hero image theme according header style #516

Conversation

yeniatencio commented Oct 1, 2024 • edited Loading

Jira

Problem/Motivation

Fix

Related PRs

Screenshots

TODO

MdNadimHossain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MdNadimHossain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeniatencio commented Oct 8, 2024

yeniatencio commented Oct 28, 2024

vincent-gao commented Nov 1, 2024

vincent-gao commented Nov 1, 2024

vincent-gao left a comment

Choose a reason for hiding this comment

vincent-gao commented Nov 21, 2024

yeniatencio commented Oct 1, 2024 •

edited

Loading